feat: Vault Transit secrets engine signer support for Stellar#801
feat: Vault Transit secrets engine signer support for Stellar#801samkirton wants to merge 2 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a ChangesStellar Vault Transit Signer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## main #801 +/- ##
==========================================
- Coverage 89.55% 89.54% -0.02%
==========================================
Files 301 302 +1
Lines 127697 127995 +298
==========================================
+ Hits 114360 114610 +250
- Misses 13337 13385 +48
🚀 New features to boost your workflow:
|
samkirton
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/services/signer/stellar/vault_transit_signer.rs (1)
41-46: Convert constructor to returnResultfor graceful error handling.
VaultTransitSigner::newpanics on missing config via.expect()at line 45. Convert it to returnResultand propagate the error through the factory methods, both of which already handleResulttypes:
stellar/mod.rs:221: Factory wraps result inOk()solana/mod.rs:278: Factory already returnsResultThis aligns with the Rust guideline: "Prefer Result over panic, use ? operator for error propagation."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/signer/stellar/vault_transit_signer.rs` around lines 41 - 46, The VaultTransitSigner::new constructor currently panics via .expect() when vault transit config is missing instead of gracefully handling the error. Change the method signature to return Result<Self, Error> (or appropriate error type), replace the .expect() call with the ? operator or match statement to propagate the error, and then update both factory methods—the one in stellar/mod.rs at line 221 that currently wraps the result in Ok() and the one in solana/mod.rs at line 278 that already returns Result—to properly handle the new Result-returning constructor by using the ? operator to propagate the error or wrapping as needed to maintain the factory's Result return type.Source: Coding guidelines
examples/stellar-vault-transit-signer/docker-compose.yaml (1)
27-29: ⚡ Quick winAdd Vault to service dependency ordering for deterministic startup.
The relayer example depends on Vault for signer auth/signing, but
depends_ononly lists Redis. Add Vault to reduce first-boot failures/race conditions in the example stack.Suggested patch
depends_on: - redis + - vault🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/stellar-vault-transit-signer/docker-compose.yaml` around lines 27 - 29, The relayer service in the docker-compose.yaml file has a depends_on section that only lists redis, but the relayer also depends on Vault for signer authentication and signing operations. Add vault to the depends_on list alongside redis to ensure Vault is started before the relayer service, preventing race conditions and first-boot failures. Update the depends_on block to include both redis and vault as dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/stellar.mdx`:
- Around line 536-538: The Vault Transit example link in the documentation
points to an incorrect path that does not match the Stellar example directory
structure added in this changeset. Update the vault-transit-signer example link
to reference the correct location of the Stellar Vault Transit example directory
so that readers are directed to the correct example code.
In `@examples/stellar-vault-transit-signer/config/config.json`:
- Around line 32-44: The `pubkey` field in the vault_transit configuration is
set to an empty string, but the `VaultTransitSignerConfig` validation in
src/models/signer/mod.rs requires it to be a non-empty, valid base64-encoded
Ed25519 public key. Replace the empty string value with a valid base64-encoded
Ed25519 public key that corresponds to the signing key specified in the
`key_name` field to satisfy the validation requirements and allow the example to
start successfully.
In `@examples/stellar-vault-transit-signer/docker-compose.yaml`:
- Around line 50-54: The Vault service in the docker-compose.yaml file is
exposing port 8200 on all host interfaces (0.0.0.0) while using a fixed
development token, creating a security vulnerability in shared/dev-network
environments. Modify the port binding in the ports section from the format that
exposes to all interfaces to one that restricts the binding to localhost only,
ensuring the Vault API is accessible only from the local machine where the
container is running.
In `@examples/stellar-vault-transit-signer/README.md`:
- Around line 38-40: The VAULT_ADDR environment variable example in the README
uses 0.0.0.0 as the host address, which is a listen/bind address and not
appropriate for client connections. Replace 0.0.0.0 with 127.0.0.1 or localhost
in the VAULT_ADDR export statement to provide a correct and reliable example
that works across different system configurations.
---
Nitpick comments:
In `@examples/stellar-vault-transit-signer/docker-compose.yaml`:
- Around line 27-29: The relayer service in the docker-compose.yaml file has a
depends_on section that only lists redis, but the relayer also depends on Vault
for signer authentication and signing operations. Add vault to the depends_on
list alongside redis to ensure Vault is started before the relayer service,
preventing race conditions and first-boot failures. Update the depends_on block
to include both redis and vault as dependencies.
In `@src/services/signer/stellar/vault_transit_signer.rs`:
- Around line 41-46: The VaultTransitSigner::new constructor currently panics
via .expect() when vault transit config is missing instead of gracefully
handling the error. Change the method signature to return Result<Self, Error>
(or appropriate error type), replace the .expect() call with the ? operator or
match statement to propagate the error, and then update both factory methods—the
one in stellar/mod.rs at line 221 that currently wraps the result in Ok() and
the one in solana/mod.rs at line 278 that already returns Result—to properly
handle the new Result-returning constructor by using the ? operator to propagate
the error or wrapping as needed to maintain the factory's Result return type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1646542d-c217-4943-94bb-4f000fce933a
📒 Files selected for processing (10)
README.mddocs/configuration/signers.mdxdocs/index.mdxdocs/stellar.mdxexamples/stellar-vault-transit-signer/.env.exampleexamples/stellar-vault-transit-signer/README.mdexamples/stellar-vault-transit-signer/config/config.jsonexamples/stellar-vault-transit-signer/docker-compose.yamlsrc/services/signer/stellar/mod.rssrc/services/signer/stellar/vault_transit_signer.rs
| For complete examples: | ||
| - Vault Transit: [vault-transit-signer example](https://github.com/OpenZeppelin/openzeppelin-relayer/tree/main/examples/vault-transit-signer) | ||
| - AWS KMS: [aws-kms-signer example](https://github.com/OpenZeppelin/openzeppelin-relayer/tree/main/examples/aws-kms-signer) |
There was a problem hiding this comment.
Fix Vault Transit example link target.
The listed Vault Transit example path does not match the Stellar example directory added in this change set, so readers will be sent to the wrong location.
Suggested patch
-- Vault Transit: [vault-transit-signer example](https://github.com/OpenZeppelin/openzeppelin-relayer/tree/main/examples/vault-transit-signer)
+- Vault Transit: [stellar-vault-transit-signer example](https://github.com/OpenZeppelin/openzeppelin-relayer/tree/main/examples/stellar-vault-transit-signer)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| For complete examples: | |
| - Vault Transit: [vault-transit-signer example](https://github.com/OpenZeppelin/openzeppelin-relayer/tree/main/examples/vault-transit-signer) | |
| - AWS KMS: [aws-kms-signer example](https://github.com/OpenZeppelin/openzeppelin-relayer/tree/main/examples/aws-kms-signer) | |
| For complete examples: | |
| - Vault Transit: [stellar-vault-transit-signer example](https://github.com/OpenZeppelin/openzeppelin-relayer/tree/main/examples/stellar-vault-transit-signer) | |
| - AWS KMS: [aws-kms-signer example](https://github.com/OpenZeppelin/openzeppelin-relayer/tree/main/examples/aws-kms-signer) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/stellar.mdx` around lines 536 - 538, The Vault Transit example link in
the documentation points to an incorrect path that does not match the Stellar
example directory structure added in this changeset. Update the
vault-transit-signer example link to reference the correct location of the
Stellar Vault Transit example directory so that readers are directed to the
correct example code.
| "config": { | ||
| "address": "http://vault:8200", | ||
| "role_id": { | ||
| "type": "env", | ||
| "value": "VAULT_ROLE_ID" | ||
| }, | ||
| "secret_id": { | ||
| "type": "env", | ||
| "value": "VAULT_SECRET_ID" | ||
| }, | ||
| "key_name": "my_signing_key", | ||
| "pubkey": "" | ||
| } |
There was a problem hiding this comment.
vault_transit example config is invalid with empty pubkey.
Line 43 sets "pubkey": "", but VaultTransitSignerConfig.pubkey is validated as non-empty and signer derivation also expects a valid base64 Ed25519 public key. This will fail config validation/startup for the example.
Suggested fix
- "pubkey": ""
+ "pubkey": "REPLACE_WITH_BASE64_ED25519_PUBLIC_KEY"Based on the provided config contract, pubkey must be present/non-empty in src/models/signer/mod.rs (VaultTransitSignerConfig validation).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "config": { | |
| "address": "http://vault:8200", | |
| "role_id": { | |
| "type": "env", | |
| "value": "VAULT_ROLE_ID" | |
| }, | |
| "secret_id": { | |
| "type": "env", | |
| "value": "VAULT_SECRET_ID" | |
| }, | |
| "key_name": "my_signing_key", | |
| "pubkey": "" | |
| } | |
| "config": { | |
| "address": "http://vault:8200", | |
| "role_id": { | |
| "type": "env", | |
| "value": "VAULT_ROLE_ID" | |
| }, | |
| "secret_id": { | |
| "type": "env", | |
| "value": "VAULT_SECRET_ID" | |
| }, | |
| "key_name": "my_signing_key", | |
| "pubkey": "REPLACE_WITH_BASE64_ED25519_PUBLIC_KEY" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/stellar-vault-transit-signer/config/config.json` around lines 32 -
44, The `pubkey` field in the vault_transit configuration is set to an empty
string, but the `VaultTransitSignerConfig` validation in
src/models/signer/mod.rs requires it to be a non-empty, valid base64-encoded
Ed25519 public key. Replace the empty string value with a valid base64-encoded
Ed25519 public key that corresponds to the signing key specified in the
`key_name` field to satisfy the validation requirements and allow the example to
start successfully.
| ports: | ||
| - 8200:8200 | ||
| environment: | ||
| - VAULT_DEV_ROOT_TOKEN_ID=dev-only-token | ||
| cap_add: |
There was a problem hiding this comment.
Restrict Vault dev API exposure to localhost.
The Vault service uses a fixed dev root token and publishes 8200 on all host interfaces. That combination is unsafe for shared/dev-network environments; bind to loopback only.
Suggested patch
vault:
image: hashicorp/vault:1.19.0
platform: linux/amd64
ports:
- - 8200:8200
+ - 127.0.0.1:8200:8200📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ports: | |
| - 8200:8200 | |
| environment: | |
| - VAULT_DEV_ROOT_TOKEN_ID=dev-only-token | |
| cap_add: | |
| ports: | |
| - 127.0.0.1:8200:8200 | |
| environment: | |
| - VAULT_DEV_ROOT_TOKEN_ID=dev-only-token | |
| cap_add: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/stellar-vault-transit-signer/docker-compose.yaml` around lines 50 -
54, The Vault service in the docker-compose.yaml file is exposing port 8200 on
all host interfaces (0.0.0.0) while using a fixed development token, creating a
security vulnerability in shared/dev-network environments. Modify the port
binding in the ports section from the format that exposes to all interfaces to
one that restricts the binding to localhost only, ensuring the Vault API is
accessible only from the local machine where the container is running.
| ```bash | ||
| export VAULT_ADDR='http://0.0.0.0:8200' | ||
| export VAULT_TOKEN='dev-only-token' |
There was a problem hiding this comment.
Use localhost for VAULT_ADDR in CLI examples.
0.0.0.0 is a listen/bind address, not the recommended client destination. This can fail on some setups; use 127.0.0.1 (or localhost) in the command example.
Suggested patch
-export VAULT_ADDR='http://0.0.0.0:8200'
+export VAULT_ADDR='http://127.0.0.1:8200'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| export VAULT_ADDR='http://0.0.0.0:8200' | |
| export VAULT_TOKEN='dev-only-token' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/stellar-vault-transit-signer/README.md` around lines 38 - 40, The
VAULT_ADDR environment variable example in the README uses 0.0.0.0 as the host
address, which is a listen/bind address and not appropriate for client
connections. Replace 0.0.0.0 with 127.0.0.1 or localhost in the VAULT_ADDR
export statement to provide a correct and reliable example that works across
different system configurations.
…y coderabbitai, increased docstring coverage for functions
samkirton
force-pushed
the
stellar-vault-transit-signer
branch
from
afbf76b to
3d1911b
Compare
1 participant
Add HashiCorp Vault Transit Support for Stellar Signing (#800)
Summary
This proposal adds HashiCorp Vault Transit support to Stellar relayers in OpenZeppelin Relayer.
Implementation
Documentation & Examples
Notes
The implementation is designed to align with the existing Vault Transit integration already available for Solana, minimising integration complexity while extending support for Stellar’s Ed25519 signing requirements.
Summary by CodeRabbit
Release Notes
New Features
Documentation